ESQL: Implement network_direction function
#136133
Conversation
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Pinging @elastic/kibana-esql (ES|QL-ui) |
|
Hi @MattAlp, I've created a changelog YAML for you. |
🔍 Preview links for changed docs |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public class NetworkDirectionUtils { |
There was a problem hiding this comment.
Despite being an utility, this class seems very specific to the esql function; it is not a general-purpose network utility. Is it possible to avoid adding it to server?
There was a problem hiding this comment.
After exploring alternatives, like placing it within libs, it seems that the dependency on CIDRUtils and InetAddresses means that moving this elsewhere would require a larger refactor/org of the networking-related utility classes.
|
Is this intended to be a snapshot function? Because if not, you'll have to add it in
|
x-pack/plugin/esql/qa/testFixtures/src/main/resources/ip.csv-spec
Outdated
Show resolved
Hide resolved
...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionTests.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirection.java
Show resolved
Hide resolved
|
I don't know if there is such a thing as aliases in ESQL scalar functions, but if there was, would be nice to only have to write |
| @Override | ||
| public String paramName(boolean blockStyle) { | ||
| return name + (blockStyle ? "Block" : "Vector"); | ||
| return name + "Block"; |
There was a problem hiding this comment.
Are you sure this is needed?
I ran ./gradlew :x-pack:plugin:esql:compileJava locally on this repo, with and without this change, and I don't see any generated classes change. I think it isn't needed.
There was a problem hiding this comment.
I believe that's because I've since changed the signature and annotations on the process method, which no longer triggers the faulty codegen.
In order to reproduce, you should be able to check out the earlier commit 3129e57, revert this change, and see what happens when you attempt to compile.
Nik should be able to confirm when he's back - as I understand, BlockArguments can't be represented as Vectors by definition, which is why the reproducer will create valid control flow but mistakenly refer to a ${name}Vector on one line while referencing the ${name}Block earlier.
There was a problem hiding this comment.
I made this change in my local copy, then ran the gradle command from earlier. The result is captured in the screenshot.
Looks like the semantics are broken since addRawVector method is called and passed a valuesBlock instead of valuesVector. Not that it matters a lot, I mean compiler is happy, and method is no-op anyways so param can be called anything. It just breaks the semantics, that's all. You're right that vectors don't mean much for the block arg class, but at least the current way both the method and its param name are in sync.
There was a problem hiding this comment.
That screenshot looks fine - though I had just asked @mouhc1ne about removing this method entirely because it's not called. Let me have a look at the rest of it.
There was a problem hiding this comment.
OK! I understand now. @MattAlp does indeed need this change. We don't have any other functions that'd bump into this. But, it does, indeed, make a lame parameter name on those methods. But, like 20 minutes ago, I requested skipping generating these methods anyway. So I guess that's fine.
It looks like there is, https://github.com/mattalp/elasticsearch/blob/24d8f307acd7ed2e249774899a94cd0135464b16/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java#L457 |
...in/java/org/elasticsearch/xpack/esql/expression/function/scalar/ScalarFunctionWritables.java
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirection.java
Show resolved
Hide resolved
...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionTests.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public String paramName(boolean blockStyle) { | ||
| return name + (blockStyle ? "Block" : "Vector"); | ||
| return name + "Block"; |
There was a problem hiding this comment.
OK! I understand now. @MattAlp does indeed need this change. We don't have any other functions that'd bump into this. But, it does, indeed, make a lame parameter name on those methods. But, like 20 minutes ago, I requested skipping generating these methods anyway. So I guess that's fine.
5 participants
network_direction(source_ip, destination_ip, networks)function based on the processor of the same nameBlockArgument.javain order to create a functioning evaluator.